Skills with a CELL pattern hit their pattern, not a single target - #2289

No actionable comments were generated in the recent review. 🎉

Walkthrough

The change parses skill cell coordinates from Skill.dat, stores them in the database, propagates them to SkillInfo, and uses them for directional area target selection. Skills without valid patterns continue to use rectangular range checks. Combo parsing also reads triplets after the leading switch.

Changes

Skill cell targeting

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟡 Moderate · up to c7661

Area skills now use persisted cell patterns, but malformed pattern data can still be accepted as a truncated pattern, potentially selecting fewer or incorrect targets without an error; two new C# files also violate the repository’s encoding requirement. Merge should wait for these issues to be corrected.

Sequence Diagram(s)

sequenceDiagram
  participant SkillParser
  participant SkillData
  participant Skill
  participant SkillResolver
  participant SkillInfo
  participant TargetResolver
  participant SkillCells
  participant MonstersAndPlayers
  SkillParser->>SkillData: read CELL and COST triples
  SkillParser->>Skill: store CellPattern
  SkillResolver->>Skill: read CellPattern
  SkillResolver->>SkillInfo: populate CellPattern
  TargetResolver->>SkillCells: parse and resolve CellPattern
  SkillCells-->>TargetResolver: return resolved cells
  TargetResolver->>MonstersAndPlayers: apply IsHit
  TargetResolver-->>TargetResolver: use WithinRange without a pattern

• Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Holding this one on placement rather than on the logic — TargetResolver and the pattern handling look right.

SkillCellTable.g.cs is 85 lines of client data checked into NosCore.GameObject, and CELL is a Skill.dat section. Everything else in that file already goes through NosCore.Parser into the database — CELL appears in SkillParser.cs only as a commented-out sample line, so it is unparsed rather than deliberately excluded.

Two problems with the table as a table:

• it cannot be regenerated from a client update without whatever produced it, which is not in the repo
• the header says the pattern spills into "the unused tail of COST" when it exceeds thirty cells, which is exactly the kind of layout knowledge the parser exists to hold

The add-dat-field route would put the pattern on the skill row and delete the generated file. Same behaviour, and it survives the next client patch.

Happy to be wrong if there is a reason CELL cannot be a column — if the pattern genuinely is not per-skill-row, say so and I will merge as is.

Taken, on both counts. SkillCellTable.g.cs is gone and CELL now goes through NosCore.Parser onto a CellPattern column, like every other section of the file.

To answer the question you left open — is the pattern per-skill-row? Yes, and it isn't a judgement call: Skill.dat has 1958 VNUM sections and 1958 CELL sections, one each, exactly like NAME and TYPE. Sixty-eight are non-empty.

The layout knowledge is now where you said it belonged. CELL is ninety-three fields: two of header, then triples (dx, dy, continues), ending at the first triple whose third field is zero. 2 + 30×3 + 1 = 93, so the section holds thirty cells at most, and a pattern needing more continues in the tail of COST — thirty-three fields of which three are read, zero for 1948 of the 1958.

And the trap inside that, which is why the tail can't simply be appended: running out of room is not the same as continuing. A continues of 1 on the last available triple means "from here it is not said", not "there is more". Skill 1175 fills all thirty with a closed figure and stops; 1857 fills them and goes on. There's a parser test for each.

The column is "dx,dy,dx,dy,...". Not a child table because the pattern is one value of the skill, read whole or not at all — nothing ever asks for its fourth cell; the longest is eighty numbers. A malformed value parses to null rather than throwing, so one bad row degrades a cast to a single target instead of taking it down.

The tests that checked the decoded data moved to SkillParserTests against a synthetic Skill.dat. What's left in GameObject is reading the column and rotating the pattern towards the target.

Zero build warnings; NosCore.Parser.Tests 105/105 (5 new), NosCore.GameObject.Tests 388/388. The Skill.dat documentation snapshot was regenerated for the new column. No play test — I don't start the servers.

Rebased and trimmed to the CLAUDE.md that landed today: comments cut back to the ones that answer a question a reader would otherwise have to dig for, and nothing outside the project named. No behaviour change in this push.

we don't use merge master into pr we use rebase please remove that merge commit and rebase